-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix false positives for display-name #2399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix false positives for display-name #2399
Conversation
@@ -90,13 +90,13 @@ module.exports = { | |||
const namedClass = ( | |||
(node.type === 'ClassDeclaration' || node.type === 'ClassExpression') && | |||
node.id && | |||
node.id.name | |||
!!node.id.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the below addition of !!
aren't strictly needed but I spotted every other variable in this block returns a boolean so I figured these should too
tests/lib/rules/display-name.js
Outdated
import React from 'react' | ||
|
||
export const ComponentWithMemoAndForwardRef = React.memo( | ||
React.forwardRef(({ world }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the thing inside forwardRef
is not a component, and should not be identified as such. forwardRef
callbacks take props and ref
, which isn't what components take.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks for the clarification :)
In that case I think there is something off with the Component detection, as this testcase identifies two components (as found by doing a console.log(components.list())
in the Program:exit
function in the display-name rule). It sounds like it should only identify one - possibly the outer call to React.memo
. This also occurs on master too.
At risk of creating one of those magic eye "how many triangles can you see puzzles", how many components should be identified in the following pieces of code?
// Currently reports 1 component - I *think* that is the result of calling React.memo, and the argument that is passed into React.memo is ignored, can you confirm?
const ComponentWithMemo = React.memo(function Component({ world }) {
return <div>Hello {world}</div>
})
// currently reports 1 component - The result of calling React.forwardRef is a component, the argument that is passed into React.forwardRef isn't a component
const ComponentWithForwardRef = React.forwardRef(function NotAComponent({ world }, ref) {
return <div>Hello {world}</div>
}, )
// Should this report one or two components found?
// Currently it seems to identify two, - The result of calling React.memo, and the result of React.forwardRef, but that seems at odds with how React.memo worked previously where it didn't count the component that was the argument passed into React.memo
const ComponentWithMemoAndForwardRef = React.memo(React.forwardRef(function Component({ world }, ref) {
return <div>Hello {world}</div>
}))
I don't understand how the internal Component identification heuristics works and what knock-on effects changing them would have. It looks like 98d4bf3 did some work relating to this but I'm not sure how to fix my above confusion
tests/lib/rules/display-name.js
Outdated
}, { | ||
// React does not currently set a displayName on the memo function when you | ||
// pass it a value from forwardRef. | ||
// Hopefully eventually that will be fixed, but until then flagging this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't be fixed because it's not broken; forwardRef is a component but does not accept one; it accepts a callback that takes props
and ref
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm referring to a slightly different issue. I'm referring how React.memo handles accepting a Component vs the result of calling forwardRef.
When inspecting the MemoFunction and MemoThenForwardRefFunction
items on https://codesandbox.io/s/awesome-paper-hh0wh?module=App.js I end up with the following in my react devtools:
Note that calling React.memo with a function component defined inline
const MemoFunction = React.memo(function MemoFunction() {
return <div>MemoFunction</div>;
});
Results in in the name MemoFunction [memo]
in the component tree.
However calling React.memo with an invocation of React.forwardRef (which returns a Component)
const MemoThenForwardRefFunction = React.memo(
React.forwardRef(function MemoThenForwardRefFunction() {
return <div>MemoThenForwardRefFunction</div>;
})
);
Results in the name Anonymous
in the component tree - Given that the result of an unwrapped call to React.forwardRef has a meaningful name, I would have expected that to have been picked up by React.memo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've raised this upstream as facebook/react#16722
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
Heya @ljharb, I've gave this a kick again and I think it is ready for review. I've updated the naming in the tests. The functions that you pass into I'm torn on what to do when we have a nested |
Removed the unneeded parser settings in tests |
@BPScott: FYI, it looks like this React issue was solved in November 2019, so it might make sense to allow this now, too 🙂 |
@fefrei please feel free to file an issue or a PR! |
Wrapping a named function declaration with a React.memo or
React.forwardRef will no longer throw false positive errors.
Check how React devtools exposes the displayNames of components wrapped in memo and forwardRef in https://codesandbox.io/s/awesome-paper-hh0wh?module=App.js
The following no longer raise a linting warning (as react can determine displayNames for them):
Using either an unnamed Function declaration (
function() {/*...*/}
) or arrow functions continue to raise a warning, as a displayName can not be inferred.Nesting memo and forwardref (
const Component = React.memo(React.forwardRef(function Component() { /*...*/}))
is not currently handled by react so continue to raise an error in that case (see facebook/react#16722)Fixes #2324, Fixes #2269